Skip to content

Conversation

@julesfll
Copy link
Collaborator

@julesfll julesfll commented Oct 27, 2020

I have the frontend for the voucher done (still need to do redux/api) and also added a variety of tweaks/fixes:

  • Fixed NestedDOM <a> warning
  • Fixed NestedDOM <td> <button> warning
  • Fixed fill-rule warning
  • Readded scrollable to make modal scroll correctly
  • Cleaned up padding css in SearchTool.js
  • Converted help text to Bootstrap component
  • Fixed overlap with sidebar and table and made it more responsive
    • Sidebar before had fixed width, changed to bootstrap breakpoints
    • There is still a gutter on the right that I need to fix that causes horizontal scrolling

demo

@julesfll
Copy link
Collaborator Author

Seems like it broke some tests, not exactly sure what though

@julesfll julesfll requested a review from billyhunt November 1, 2020 20:29
Copy link
Collaborator

@billyhunt billyhunt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! Maybe fix some of the stuff I mentioned if you feel like it.

1
}
onSetPage={(index) => props.pagination.page = index}
onSetPage={(index) => (props.pagination.page = index)}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what is going on here. You are setting the prop to index? Should you be setting state?

Copy link
Collaborator Author

@julesfll julesfll Nov 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not exactly sure- maybe it's intended as a Redux update? Was someone working on fixing the pagination? Otherwise I can look into this

<Col xs="10" style={{ paddingLeft: "15px" }}>
Pets allowed
</Col>
<Col xs="10">Pets allowed</Col>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did it not need the padding anymore?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved the padding "up a level"- the class name px-3 adds the padding to the parent instead of each child having their own padding

>
<path
fill-rule="evenodd"
fillRule="evenodd"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this doing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure, I think it has to do with SVG formats; just changed it because it was throwing a warning (because React prefers camel-cased params)

@billyhunt
Copy link
Collaborator

Looks like you have to update your snapshots

@julesfll
Copy link
Collaborator Author

julesfll commented Nov 7, 2020

Looks like you have to update your snapshots

Oh ok! How would I do that?

@billyhunt
Copy link
Collaborator

Sorry I missed this. Did you get it figured out? Slack me tomorrow if not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants